-
-
Notifications
You must be signed in to change notification settings - Fork 396
Deduplicate and reorganize specs for Kernel#raise #1332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
8d4fdf6 to
1e957b6
Compare
| cause = StandardError.new | ||
| -> { raise("error", cause: cause) }.should raise_error(RuntimeError) { |e| e.cause.should == cause } | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: haven't found a similar test in :kernel_raise_with_cause. Isn't it just being deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems I had code blindness when reviewing changes. I've added tests for this and the next one. Also added a context "without cause keyword argument", which now houses tests for what happens when cause is not specified.
|
|
||
| it "doesn't raise a TypeError when given cause is nil" do | ||
| -> { raise "message", cause: nil }.should raise_error(RuntimeError, "message") | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: similar issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment
| raise | ||
| end | ||
| end.should raise_error(StandardError, "aaa") | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: the same issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is covered by existing https://github.com/ruby/spec/pull/1332/changes#diff-866cdb19d113cd7e3798855d9e2bc74a30e2307efae414383ee88629580d80a5R20-R39
- Include shared spec which has better descriptions and more tests. - Test Kernel#raise, not Kernel.raise (Kernel.raise has separate tests). - Remove duplicated tests. There is more work to be done in organizing the tests: - Fiber and Thread seem to have some generic tests that are already included in the shared spec. - Tests for intricacies of re-raising exceptions could also be moved to the shared spec probably.
1e957b6 to
de25fdd
Compare
|
How did a test that doesn't exist on this branch fail on an Encoding that doesn't exist in Ruby? 😅 |
|
^ discussed in #1330 (comment) |
There is more work to be done in organizing the tests: